Lazy napari checks for BatchRunner to improve import times #5
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR optimizes import times by deferring napari imports until they are actually needed. The key change is moving napari availability checking from module load time to runtime, reducing startup overhead when napari is not immediately required.
Key Changes:
- Implements lazy loading for napari via a cached
_check_napari()function - Renames
HAS_NAPARIto_HAS_NAPARIto indicate it's a private implementation detail - Moves the
create_workerimport from module-level into the_run_napari_threadedmethod
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/nbatch/_runner.py | Implements lazy napari checking with _check_napari() function and defers create_worker import to point of use |
| tests/test_runner.py | Updates test to reference renamed _HAS_NAPARI variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/nbatch/_runner.py
Outdated
| self._was_cancelled = True # Set immediately for threaded cases | ||
| # Store local reference to avoid race condition with _handle_finished | ||
| worker = self._worker if HAS_NAPARI else None | ||
| worker = self._worker if _check_napari() else None |
There was a problem hiding this comment.
The logic here is incorrect. It checks if napari is currently available to determine if the worker is a napari worker, but the worker type depends on what was available when run() was called, not what's available now. If napari becomes unavailable between run() and cancel() (or vice versa), this will fail.
Instead, check if the worker has a quit() method:
worker = self._worker if self._worker is not None and hasattr(self._worker, 'quit') else None| worker = self._worker if _check_napari() else None | |
| worker = self._worker if self._worker is not None and hasattr(self._worker, 'quit') else None |
src/nbatch/_runner.py
Outdated
| def _check_napari() -> bool: | ||
| """Lazily check for napari availability.""" | ||
| global _HAS_NAPARI | ||
| if _HAS_NAPARI is None: | ||
| try: | ||
| import napari.qt.threading # noqa: F401 | ||
|
|
||
| _HAS_NAPARI = True | ||
| except ImportError: | ||
| _HAS_NAPARI = False | ||
| return _HAS_NAPARI |
There was a problem hiding this comment.
Race condition: Multiple threads could simultaneously check _HAS_NAPARI is None and both attempt to import napari. While this won't cause incorrect behavior (both will set the same value), it's inefficient and could cause multiple import attempts.
Consider using a lock or threading.Lock() to ensure thread-safe initialization:
_NAPARI_LOCK = threading.Lock()
def _check_napari() -> bool:
"""Lazily check for napari availability."""
global _HAS_NAPARI
if _HAS_NAPARI is None:
with _NAPARI_LOCK:
# Double-check after acquiring lock
if _HAS_NAPARI is None:
try:
import napari.qt.threading # noqa: F401
_HAS_NAPARI = True
except ImportError:
_HAS_NAPARI = False
return _HAS_NAPARI
tests/test_runner.py
Outdated
| import nbatch._runner as runner_module | ||
|
|
||
| monkeypatch.setattr(runner_module, 'HAS_NAPARI', False) | ||
| monkeypatch.setattr(runner_module, '_HAS_NAPARI', False) |
There was a problem hiding this comment.
Potential test issue: The test monkeypatches _HAS_NAPARI to force the fallback path, but this only works if _check_napari() hasn't been called yet. If any prior test already called _check_napari(), the cached value would be used and this monkeypatch would have no effect.
To make this test more robust, also patch the _check_napari function itself:
monkeypatch.setattr(runner_module, '_HAS_NAPARI', False)
monkeypatch.setattr(runner_module, '_check_napari', lambda: False)Or reset the cache before patching:
runner_module._HAS_NAPARI = None
monkeypatch.setattr(runner_module, '_HAS_NAPARI', False)| monkeypatch.setattr(runner_module, '_HAS_NAPARI', False) | |
| monkeypatch.setattr(runner_module, '_HAS_NAPARI', False) | |
| monkeypatch.setattr(runner_module, '_check_napari', lambda: False) |
1 participant
This pull request refactors how the code detects and handles the presence of the
naparilibrary for threaded batch operations. Instead of checking fornapariat module import time, the code now tries to importnaparionly when needed, improving flexibility and testability. The test suite is also updated to simulate the absence ofnapariby monkeypatching the import mechanism.Refactoring napari detection and threading logic:
HAS_NAPARIflag and deferred importingnaparito runtime, allowing the code to try using napari threading if available and fall back to standard threading otherwise (src/nbatch/_runner.py) [1] [2].cancelmethod to check for aquitmethod on the worker instead of relying on theHAS_NAPARIflag, making cancellation logic more robust and decoupled from napari-specific checks (src/nbatch/_runner.py).create_workerinto the_run_napari_threadedmethod so it only occurs when napari threading is actually used (src/nbatch/_runner.py).Testing improvements:
tests/test_runner.py) [1] [2].